-
Notifications
You must be signed in to change notification settings - Fork 227
[APIView] Identify the package type #12398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces package type classification functionality to the API review system. The main purpose is to enable categorization of packages as Data plane, Management plane, or Unknown types throughout the review workflow.
Key changes include:
- Added PackageType enum and supporting infrastructure for package classification
- Enhanced review creation and update workflows to handle package type assignment
- Improved error handling and code organization in the review page component
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dotnet/APIView/ClientSPA/src/app/_models/review.ts | Added PackageType enum and optional packageType field to Review class |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.ts | Added updateReview method, improved error handling, and consolidated review update logic |
| src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs | Added packageType parameter to CreateReviewAsync and new UpdateReviewAsync method |
| src/dotnet/APIView/APIViewWeb/Managers/PullRequestManager.cs | Enhanced pull request workflow to support package type classification |
| src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IReviewManager.cs | Updated interface signatures for package type support |
| src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IPullRequestManager.cs | Added packageType parameter to interface method |
| src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs | Added PackageType property to ReviewListItemModel |
| src/dotnet/APIView/APIViewWeb/LeanControllers/ReviewsController.cs | Added blank lines (formatting change) |
| src/dotnet/APIView/APIViewWeb/LeanControllers/PullRequestsController.cs | Added packageType parameter to API endpoint |
| src/dotnet/APIView/APIViewWeb/Helpers/CommonUtilities.cs | Defined PackageType enum with Data, Management, and Unknown values |
| src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs | Enhanced auto-review workflow with package type classification support |
src/dotnet/APIView/APIViewWeb/LeanControllers/ReviewsController.cs
Outdated
Show resolved
Hide resolved
…r.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tjprescott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.ts
Outdated
Show resolved
Hide resolved
tjprescott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but would like sign-off from @praveenkuttappan since he has more knowledge of the C# side of APIView than me.
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.ts
Outdated
Show resolved
Hide resolved
maririos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add tests to make sure the package type is correctly populated in all types of APIViews creations?
praveenkuttappan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
src/dotnet/APIView/APIViewWeb/Controllers/AutoReviewController.cs
Outdated
Show resolved
Hide resolved
maririos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ensure that there are tests covering:
- Passing a valid
packageType - Passing an invalid
packageType - Omitting
packageType - Updating an existing review with and without
PackageTypeset
Closes #12242